Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dependency injection #964

Merged
merged 36 commits into from
Oct 19, 2024
Merged

Dependency injection #964

merged 36 commits into from
Oct 19, 2024

Conversation

sezanzeb
Copy link
Owner

@sezanzeb sezanzeb commented Oct 2, 2024

Blocked by #960

  • Further simplifies the test setup, because instead of having to clean up global singletons, they are created from scratch in each test to the testers hearts desire.
  • Importing files doesn't have side-effects.
  • Dependencies can be replaced by MagicMock objects, narrowing down the scope of unit-tests.

Not all classes are transitioned to this pattern. See comment below.

@sezanzeb sezanzeb changed the base branch from main to improved_test_setup October 2, 2024 14:33
@sezanzeb sezanzeb force-pushed the dependency-injection branch from ebb0145 to e0a8127 Compare October 3, 2024 13:45
@sezanzeb sezanzeb force-pushed the dependency-injection branch from f8f21ce to 166792b Compare October 3, 2024 13:58
@sezanzeb
Copy link
Owner Author

sezanzeb commented Oct 3, 2024

doing dependency injection for macros is rather difficult to do. In the end, the Mapping objects need the OutputParser (a class wrapping the def parse method and the others of that file) injected, which causes problems when serializing them. I haven't looked into it in more detail yet, as I didn't want to fiddle around more than I already did at that point.

But a little bit is done in this branch at least.

@sezanzeb sezanzeb mentioned this pull request Oct 16, 2024
@sezanzeb sezanzeb force-pushed the dependency-injection branch from 076b93e to a113698 Compare October 16, 2024 20:16
@sezanzeb sezanzeb marked this pull request as ready for review October 16, 2024 20:59
@sezanzeb sezanzeb requested a review from jonasBoss October 16, 2024 20:59
@sezanzeb sezanzeb changed the base branch from improved_test_setup to main October 16, 2024 20:59
Copy link
Collaborator

@jonasBoss jonasBoss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the changes, i think it simplifies things and makes it easier to test

@sezanzeb sezanzeb merged commit 057663a into main Oct 19, 2024
6 checks passed
@sezanzeb
Copy link
Owner Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants